Skip to content

fix: deployments#76

Merged
da-bao-jian merged 3 commits intomainfrom
fix-deployment
May 5, 2025
Merged

fix: deployments#76
da-bao-jian merged 3 commits intomainfrom
fix-deployment

Conversation

@will-luban
Copy link
Copy Markdown
Contributor

@will-luban will-luban commented May 4, 2025

https://linear.app/luban-protocol/issue/LIN-39/update-taiyi-based-on-the-latest-linglong-changes-with-slashing

Summary by CodeRabbit

  • New Features

    • Added explicit initialization of the EigenLayer middleware address during contract setup.
    • Introduced configuration options for delegation withdrawal delay and semantic versioning in deployment configs.
    • Added a new test contract simulating full deployment with controlled keys and funds.
  • Improvements

    • Enhanced permission and admin setup during registry and contract initialization.
    • Updated naming for operator commission parameters in configuration for clarity.
    • Adjusted execution order of deployment scripts for improved setup flow.
    • Made deployment scripts use updated deployment data paths and include middleware address in initialization.
  • Bug Fixes

    • Improved validation to prevent setting the EigenLayer middleware address to zero.

@coderabbitai
Copy link
Copy Markdown

coderabbitai bot commented May 4, 2025

Walkthrough

This set of changes updates deployment and setup scripts, a configuration file, and the TaiyiRegistryCoordinator smart contract to integrate the EigenLayerMiddleware address directly into the initialization process. The deployment scripts and contract initialization flows are updated to require and assign the middleware address. The configuration JSON introduces new parameters and renames an existing key. The order of script execution in the shell script is adjusted. Additionally, a new test contract simulates the full deployment process with environment setup and funding. These changes collectively enhance the initialization and configuration process, ensuring that the middleware dependency is established at deployment.

Changes

File(s) Change Summary
script/Deployments.s.sol, script/SetRegistry.s.sol, script/SetupContract.s.sol Updated the path for the EigenLayer deployment data JSON file to SLASHING_deploy_from_scratch_deployment_data.json. Added an extra constructor argument (the EigenLayerMiddleware proxy address) to TaiyiRegistryCoordinator initialization. Changed getPrivateKeys visibility to public. The registry script now derives an additional deployer address, adds multiple admins to PermissionController, replaces AllocationManager.setAVSRegistrar with a permissioned setAppointee call, and updates AVS metadata URI.
src/operator-registries/TaiyiRegistryCoordinator.sol Modified the initialize function to include a new _eigenLayerMiddleware address parameter. If non-zero, assigns it to the contract’s state variable eigenLayerMiddleware. Updated function signature accordingly.
script/configs/eigenlayer-deploy-config-devnet.json Added "withdrawal_delay_blocks": 900 under delegation. Renamed "default_operator_split_bips" to "global_operator_commission_bips" with the same value. Added a top-level "semver": "v0.0.0" field.
script/setup-contract.sh Changed execution order to run SetRegistry before SetupContract. Removed previously commented-out SetupContract call at the top and added a new call after SetRegistry with the same parameters.
script/configs/deploy-test-config.json Added new JSON config with detailed deployment parameters for a test environment, including maintainer, multisig addresses, strategy settings, initial paused statuses, withdrawal delays, rewards coordinator timing and commission parameters, and semantic versioning.
test/DeployTest.t.sol Added a new Forge test contract that sets environment variables for private keys and network, funds derived addresses, and sequentially runs the deployment scripts Deploy, SetRegistry, and SetupContract to simulate deployment in a controlled test environment.
test/EigenLayerMiddleware.t.sol, test/SymbioticMiddleware.t.sol Updated the initialization call for the TaiyiRegistryCoordinator proxy contract to include the EigenLayerMiddleware address as an additional argument in the constructor call during test deployments.

Sequence Diagram(s)

sequenceDiagram
    participant Deployer
    participant TaiyiRegistryCoordinator
    participant EigenLayerMiddleware

    Deployer->>TaiyiRegistryCoordinator: initialize(owner, pausedStatus, allocationManager, eigenLayerMiddleware, pauserRegistry)
    TaiyiRegistryCoordinator->>EigenLayerMiddleware: (assign middleware address if non-zero)
    TaiyiRegistryCoordinator-->>Deployer: Initialization complete
Loading
sequenceDiagram
    participant Script
    participant PermissionController
    participant EigenLayerMiddleware
    participant AllocationManager

    Script->>PermissionController: addAdmin(proxyDeployer)
    Script->>PermissionController: addAdmin(implDeployer)
    Script->>PermissionController: setAppointee(EigenLayerMiddleware, permissions)
    Script->>AllocationManager: setAVSRegistrar(EigenLayerMiddleware)
    Script->>AllocationManager: updateAVSMetadataURI("luban-local-test")
Loading

Possibly related PRs

  • fix: update deployments to the latest version #23: Refactors deployment process and updates TaiyiRegistryCoordinator initialization with middleware address, closely related to these deployment script and contract changes.
  • chore: refactor #14: Introduces and modifies the EigenLayerMiddleware contract and interface, which is now required during initialization.
  • fix revert commit #42: Also configures TaiyiRegistryCoordinator with the EigenLayerMiddleware address, focusing on middleware integration during deployment.

Suggested reviewers

  • da-bao-jian

Poem

In scripts and contracts, changes bloom,
Middleware finds its proper room.
JSON paths and configs align,
Permissions set in tidy line.
A rabbit hops with joyful cheer,
For initialization’s crystal clear!
🐇✨


Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

Support

Need help? Create a ticket on our support page for assistance with any issues or questions.

Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments.

CodeRabbit Commands (Invoked using PR comments)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai generate sequence diagram to generate a sequence diagram of the changes in this PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

@will-luban will-luban requested a review from da-bao-jian May 4, 2025 13:05
Copy link
Copy Markdown

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🔭 Outside diff range comments (1)
src/operator-registries/TaiyiRegistryCoordinator.sol (1)

111-118: 🛠️ Refactor suggestion

⚠️ Potential issue

initialize() lost pauser-registry initialisation – proxy will operate with a zero registry

Pausable’s constructor is executed only on the implementation contract, not on the proxy.
Because the _pauserRegistry argument is now commented-out and never forwarded, the storage slot pauserRegistry in the proxy remains the zero-address.
Any future pause/unpause attempts will revert, and _setPausedStatus called two lines earlier is presently operating without an actual registry.

Suggested minimal fix (keeps the parameter index intact):

-        address /* _pauserRegistry */
+        address _pauserRegistry

and after line 133:

+        require(_pauserRegistry != address(0), "Pauser registry cannot be zero address");
+        pauserRegistry = IPauserRegistry(_pauserRegistry);

This restores functional pausing while retaining backward-compatible ABI ordering.

🧹 Nitpick comments (3)
src/operator-registries/TaiyiRegistryCoordinator.sol (1)

125-135: Guardrail: make _allocationManager non-optional or add an explicit default

Leaving allocationManager unset when _allocationManager == address(0) pushes the onus on callers and silently produces a semi-initialised contract that will later revert in registerOperator.
Either:

  1. Require a non-zero address up-front,
-        if (_allocationManager != address(0)) {
-            allocationManager = IAllocationManager(_allocationManager);
-        }
+        require(_allocationManager != address(0), "AllocationManager cannot be zero");
+        allocationManager = IAllocationManager(_allocationManager);

or expose a public setter callable only-once by the owner.

script/SetRegistry.s.sol (2)

74-76: implDeployer derivation is fine – remember it’s unused below

implDeployer is computed but later never referenced outside the second addAdminToPermissionController call (see next comment).
If keeping the variable, consider unchecked{} wrapping or a short comment to explain the intent.


121-127: Hard-coding createOperatorSets.selector couples the script to contract internals

Passing a raw selector ties the deployment script to a specific function signature; any future change will silently break permissions.
Prefer reading the selector from the compiled ABI or exposing a named constant in the contract and importing it here.

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 43a2b82 and e87ea36.

📒 Files selected for processing (6)
  • script/Deployments.s.sol (2 hunks)
  • script/SetRegistry.s.sol (3 hunks)
  • script/SetupContract.s.sol (1 hunks)
  • script/configs/eigenlayer-deploy-config-devnet.json (3 hunks)
  • script/setup-contract.sh (1 hunks)
  • src/operator-registries/TaiyiRegistryCoordinator.sol (2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
  • GitHub Check: Foundry Project
🔇 Additional comments (9)
script/setup-contract.sh (1)

17-23: Updated script execution order to match dependency flow.

The script now properly runs SetRegistry before SetupContract, which aligns with the dependency flow where the registry needs to be set up before contracts are initialized.

script/SetupContract.s.sol (1)

91-95: Updated deployment data file path to include SLASHING context.

The file path has been updated to refer to the new SLASHING-specific deployment data, which aligns with the integration of slashing mechanisms mentioned in LIN-39.

script/configs/eigenlayer-deploy-config-devnet.json (3)

37-37: Added withdrawal delay parameter for delegation.

A new parameter withdrawal_delay_blocks has been added with a value of 900 (approximately 3 hours at 12-second block time), which provides a security window for withdrawals.


51-51: Renamed parameter to align with EigenLayer naming conventions.

The parameter default_operator_split_bips has been renamed to global_operator_commission_bips to better reflect its purpose and maintain consistency with EigenLayer's terminology.


60-61: Added semantic versioning field.

A new field semver has been added to track the version of the configuration, starting at v0.0.0. This helps with configuration management and identifying compatible versions.

script/Deployments.s.sol (2)

108-112: Updated deployment data file path to include SLASHING context.

The file path has been updated to refer to the SLASHING-specific deployment data, maintaining consistency with the changes in SetupContract.s.sol.


210-216: Added EigenLayerMiddleware address to TaiyiRegistryCoordinator initialization.

The TaiyiRegistryCoordinator now requires the EigenLayerMiddleware address during initialization, establishing the required dependency for slashing mechanisms.

src/operator-registries/TaiyiRegistryCoordinator.sol (1)

82-85: onlyEigenLayerMiddleware revert string is inconsistent & typo-prone

The error selector OnlyEigenlayerMiddleware() spells “Eigenlayer” with a lowercase “l”, whereas the variable and the protocol name elsewhere use “EigenLayer”.
Although purely cosmetic, mismatched revert selectors make downstream decoding/debugging harder.

-require(msg.sender == eigenLayerMiddleware, OnlyEigenlayerMiddleware());
+require(msg.sender == eigenLayerMiddleware, OnlyEigenLayerMiddleware());

Consider regenerating the custom error (or renaming the existing one) to keep casing consistent across the code-base.

Likely an incorrect or invalid review comment.

script/SetRegistry.s.sol (1)

119-120: acceptAdmin() caller/parameter order looks suspicious

PermissionController.acceptAdmin() is usually invoked by the pending admin without arguments (OE-style two-step admin).
Here it’s called by proxyDeployer and passes eigenLayerMiddleware as parameter.
Confirm the signature:

function acceptAdmin(address newAdmin) external; // ?

If the function expects msg.sender to be the new admin, this call will revert.

Copy link
Copy Markdown

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🧹 Nitpick comments (3)
test/SymbioticMiddleware.t.sol (1)

457-459: Middleware parameter added to initialization call

The addition of the EigenLayer middleware address parameter to the initialization call is consistent with the updated TaiyiRegistryCoordinator contract requirements.

However, using eigenLayerPauserReg() as a placeholder for a middleware address when it's "not useful in symbiotic context" is potentially misleading and could cause confusion for future developers.

Consider creating a dedicated mock middleware contract for testing purposes instead of repurposing an unrelated contract address. This would make the test more explicit about its intentions and better represent the actual system architecture.

test/DeployTest.t.sol (1)

1-42: New test for deployment flow verification

This is a well-structured test that validates the full deployment process by executing the deployment scripts in sequence. It correctly sets up the test environment with appropriate funding for the deployment addresses.

However, there are some security and testing practice concerns:

  1. The test includes hardcoded private keys (even if they're test keys).
  2. The test lacks assertions to verify successful deployment outcomes.

Consider:

  1. Using dynamically generated keys for tests rather than hardcoded values.
  2. Adding verification assertions after deployment to confirm contracts were deployed correctly.
  3. Adding verification that deployed contracts have expected addresses and configurations.

Example improvement:

 function testRunDeployScript() public {
+    // Generate random keys for testing
+    uint256 proxyOwnerKey = uint256(keccak256(abi.encodePacked("proxy_owner", block.timestamp)));
+    uint256 implOwnerKey = uint256(keccak256(abi.encodePacked("impl_owner", block.timestamp)));
+    
     vm.setEnv(
         "PROXY_OWNER_PRIVATE_KEY",
-        "c5114526e042343c6d1899cad05e1c00ba588314de9b96929914ee0df18d46b2"
+        vm.toString(proxyOwnerKey)
     );
     /* rest of the function... */
     
     deploy.run("deploy-test-config.json");
     setRegistry.run();
     setupContract.run();
+    
+    // Verify deployment outcomes
+    // Example: verify contract addresses from deployment data file
+    string memory deployDataPath = "script/output/devnet/SLASHING_deploy_from_scratch_deployment_data.json";
+    vm.assume(vm.exists(deployDataPath));
+    string memory deployData = vm.readFile(deployDataPath);
+    // Add assertions to verify key contracts were deployed properly
 }
script/configs/deploy-test-config.json (1)

1-62: New deployment configuration for testing

The configuration file provides a comprehensive set of parameters for the system deployment in test mode.

Several aspects of this configuration warrant attention:

  1. Using the same multisig address (0xD8F3183DEF51A987222D845be228e0Bbb932C222) for all roles would not be appropriate in production.

  2. The strategy deposit limits are set to the maximum uint256 value, which may not realistically test limit handling.

  3. The WETH token address appears to be a test address but lacks clear documentation.

Consider adding documentation comments to the JSON file explaining the purpose of each major configuration section, particularly for test-specific values that differ from production settings. This would improve clarity for developers using this configuration.

Example JSON with comments (in a separate documentation file since JSON doesn't support comments):

# deploy-test-config.json Documentation

## Multisig Addresses
All roles use the same address in test environment, but would be different in production.

## Strategies
Max deposit values are set to maximum to avoid constraints during testing.

## Timing Parameters
Most delays are minimized for faster test execution.
🧰 Tools
🪛 Gitleaks (8.21.2)

12-12: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.

(generic-api-key)

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro (Legacy)

📥 Commits

Reviewing files that changed from the base of the PR and between e87ea36 and a68b07b.

📒 Files selected for processing (6)
  • script/Deployments.s.sol (3 hunks)
  • script/configs/deploy-test-config.json (1 hunks)
  • src/operator-registries/TaiyiRegistryCoordinator.sol (2 hunks)
  • test/DeployTest.t.sol (1 hunks)
  • test/EigenLayerMiddleware.t.sol (1 hunks)
  • test/SymbioticMiddleware.t.sol (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
  • src/operator-registries/TaiyiRegistryCoordinator.sol
  • script/Deployments.s.sol
🧰 Additional context used
🪛 Gitleaks (8.21.2)
script/configs/deploy-test-config.json

12-12: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.

(generic-api-key)

⏰ Context from checks skipped due to timeout of 90000ms (1)
  • GitHub Check: Foundry Project

@da-bao-jian da-bao-jian merged commit d610ec1 into main May 5, 2025
2 checks passed
@da-bao-jian da-bao-jian deleted the fix-deployment branch May 5, 2025 14:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants